Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] New syntax #611

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[RFC] New syntax #611

wants to merge 3 commits into from

Conversation

rodjek
Copy link
Owner

@rodjek rodjek commented Sep 22, 2017

I'm planning on introducing a new syntax in rspec-puppet 3.0.0 that encourages the use of standard rspec matchers where possible. If you're using serverspec for your acceptance tests, this will look pretty familiar.

I'm just in the planning and experimenting stage at the moment, and I'd love to hear any suggestions or feedback from the users rspec-puppet. The main change so far is making the resources in the catalogue the subjects of the test, rather than the catalogue itself.

Where currently, you might have a test like this:

describe 'mysql::service' do
  it { is_expected.to contain_service('mysql').with_ensure('running').with_enabled(true) }
end

It could instead be expressed using standard rspec matchers as:

describe 'mysql::service' do
  describe service('mysql') do
    its(:parameters) { is_expected.to include(:ensure => 'running', :enable => true) }
  end
end

By creating an expectation on an aspect of the resource, you're implicitly expecting that the resource is present in the catalogue, but you can explicitly specify this expectation by using the standard exist matcher.

describe service('mysql') do
  it { is_expected.to exist }
end

Or in the negative:

describe service('mysql') do
  it { is_expected.not_to exist }
end

The relationships between resources is still implemented with custom matchers, but they are much more lightweight than they are currently.

describe service('mysql') do
  it { is_expected.to require file('/etc/mysql/mysql.conf') }
end

By making these distinct expectations, you can now easily test negatives (for example, that a resource does not subscribe to another resource) and compound expectations:

describe service('mysql') do
  it { is_expected.to require(file('/etc/mysql/mysql.conf')).or require(file('/etc/mysql.conf')) }
end

As I said at the start, nothing here is set in stone and all suggestions are welcome and will be considered. Whatever the new syntax ends up being, it will work along side the existing syntax, so none of your existing tests will suddenly break. I do plan on deprecating the current syntax in a future major release and eventually removing it in an even later major release, but it won't be any time soon.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 91.612% when pulling 3b0c8c2 on new_syntax into 11f93e7 on master.

@cyberious
Copy link

cyberious commented Oct 4, 2017

While I understand what you are trying to do, I believe the verbosity you are adding will actually lead to more code and confusion. For instance the its(:parameters) syntax could just as easily be the similar older pattern without the verbosity of its(:parameters) block. Perhaps a similar pattern to what has been adopted by serverspec in that we have it { is_expected.to have_ensure('running') } or multiple values with something like it { is_expected.to have( {:ensure => 'running' , :enabled => true }) }

@logicminds
Copy link

logicminds commented Oct 25, 2017

How would this new format work with method chaining?

it do
  is_expected.to contain_file('/tmp/test').with({ ensure: present}).that_comes_before("File['/tmp']")
end

@rodjek
Copy link
Owner Author

rodjek commented Nov 8, 2017

@cyberious 👍 I like it

@logicminds something like this

describe file('/tmp/test') do
  its(:parameters) { is_expected.to include(ensure: 'present') }
  it { is_expected.to come_before(file('/tmp')) }
end

One of the main ideas behind the new syntax is to discourage the use of long chains of methods in the expectations and instead make it easy to do small, discrete expectations on a resource. This leads to much easier to read tests and output

my::class
  File[/tmp/test]
    should be in the catalogue
    should come before File[/tmp]
    parameters
      should include {:ensure => 'present'}

@DavidS
Copy link
Collaborator

DavidS commented Nov 9, 2017

@rodjek adjacent to this, a question on Slack was raised on how to test for global properties of the catalog, like, "no directory with a source should have recurse set to true". Do you see a possibility to integrate such functionality here?

@rodjek
Copy link
Owner Author

rodjek commented Nov 9, 2017

@DavidS Yep, that's similar to another issue #229 where the user wants to be able to check that all cron resources have certain IO redirects in their command string. What I'm thinking is a way to enumerate resources of a particular type, so maybe something like this?

resources(:file).select { |f| f.ensure == 'directory' && f.source }.each do |file_resource|
  describe file_resource do
    it { is_expected.to have_attributes(:recurse => false) }
  end
end

@DavidS
Copy link
Collaborator

DavidS commented Nov 10, 2017

It reminds me of (global) property-based testing:

describe resource(:ifile).select { ... } do
  all { are_expected.to ... }
end

@rodjek
Copy link
Owner Author

rodjek commented Nov 13, 2017

Yeah, it could be written as:

describe resources(:file).select { |f| f.ensure == 'directory' && f.source } do
  it { is_expected.to all(have_attributes(:recurse => false)) }
end

That would give slightly less easy to understand output in documentation format, but a more consise test.

@ekohl
Copy link
Contributor

ekohl commented Sep 10, 2018

How would this deal with resources within a module? given the current double underscore I'd expect describe concat__fragment('x') do ... end

Another thing I'd like to see is easy verification of the full content. Currently we have helper functions:

def verify_concat_fragment_contents(subject, title, expected_lines)
  is_expected.to contain_concat__fragment(title)
  content = subject.resource('concat::fragment', title).send(:parameters)[:content]
  expect(content.split("\n") & expected_lines).to match_array(expected_lines)
end

def verify_concat_fragment_exact_contents(subject, title, expected_lines)
  is_expected.to contain_concat__fragment(title)
  content = subject.resource('concat::fragment', title).send(:parameters)[:content]
  expect(content.split(/\n/).reject { |line| line =~ /(^#|^$|^\s+#)/ }).to match_array(expected_lines)
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants